Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(lint): address existing linter issues #1218

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ecrupper
Copy link
Contributor

A few things of note:

  • I ditched dupl. This rule was really only pointing out false positives. I suppose you could argue there could be some convoluted abstraction to make step and service not copy so much code, but I don't think that would be worth it. It's really just noise right now.
  • int32 for DB types does not play well with int. int will expand based on system, so converting it to int32 does not make much sense, especially when we are using INTEGER for the DB table declaration for both int32 and int64. So I just made all int convert to int64 for the DB.
  • contextcheck was pretty particular about using database.FromContext(c) from inside helper functions, so I just grabbed the interface and passed that along instead.
  • uint64 for Starlark exec limit cannot be translated to a sql type, so I moved the conversion for that to right before we pass it along to Starlark.

golang-ci lint ✅ 🤞

@ecrupper ecrupper requested a review from a team as a code owner October 29, 2024 19:14
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 69.53125% with 39 lines in your changes missing coverage. Please review.

Project coverage is 56.87%. Comparing base (f463fc9) to head (52d535d).

Files with missing lines Patch % Lines
api/webhook/post.go 0.00% 22 Missing ⚠️
compiler/template/starlark/render.go 60.00% 4 Missing and 2 partials ⚠️
mock/server/authentication.go 0.00% 4 Missing ⚠️
api/build/enqueue.go 0.00% 2 Missing ⚠️
compiler/types/pipeline/ruleset.go 90.00% 1 Missing and 1 partial ⚠️
api/repo/repair.go 0.00% 1 Missing ⚠️
compiler/template/starlark/starlark.go 0.00% 1 Missing ⚠️
internal/token/generate_rsa.go 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1218   +/-   ##
=======================================
  Coverage   56.87%   56.87%           
=======================================
  Files         599      599           
  Lines       32856    32869   +13     
=======================================
+ Hits        18687    18695    +8     
- Misses      13535    13538    +3     
- Partials      634      636    +2     
Files with missing lines Coverage Δ
api/admin/service.go 0.00% <ø> (ø)
api/admin/step.go 0.00% <ø> (ø)
api/admin/user.go 0.00% <ø> (ø)
api/log/create_service.go 0.00% <ø> (ø)
api/log/create_step.go 0.00% <ø> (ø)
api/log/delete_service.go 0.00% <ø> (ø)
api/log/delete_step.go 0.00% <ø> (ø)
api/log/get_service.go 0.00% <ø> (ø)
api/log/get_step.go 0.00% <ø> (ø)
api/log/update_service.go 0.00% <ø> (ø)
... and 59 more

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we remove this too, then?

- dupl


globals, err := starlark.ExecFileOptions(syntax.LegacyFileOptions(), thread, "templated-base", tmpl, predeclared)
Copy link
Contributor

@plyr4 plyr4 Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im assuming its not breaking, but how impactful is this change?

globals, err := starlark.ExecFileOptions(syntax.LegacyFileOptions(), thread, "templated-base", tmpl, predeclared)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/google/starlark-go/blob/1207426daebd7ea8eb86b8d917a344a72cc5167b/starlark/eval.go#L333

It's just using what the now deprecated function has. I think we can investigate turning some of those options on, but they're all set to false right now

Copy link
Contributor

@plyr4 plyr4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome

Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants